-
Notifications
You must be signed in to change notification settings - Fork 931
(#3804, #1901) Add source locations to package information #3801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
1239d46
to
644d0ba
Compare
public bool IsPinned { get; set; } | ||
public string ExtraInformation { get; set; } | ||
public string DeploymentLocation { get; set; } | ||
public string PackageInstalledFrom { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming things is hard... Should this perhaps be SourceInstalledFrom
? We can't necessarily use Uri as it may not be a Uri? Perhaps PackageOriginationSource
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pauby can you provide any input here? This is with regard to capturing which specific source a package was downloaded and installed from. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SourceInstalledFrom
is clear what it contains, so I'd suggest that.
src/chocolatey/infrastructure.app/services/ChocolateyPackageInformationService.cs
Outdated
Show resolved
Hide resolved
{ | ||
} | ||
|
||
public PackageResult(IPackageMetadata packageMetadata, IPackageSearchMetadata packageSearch, string installLocation, string source, string sourceUri) : this(packageMetadata.Id, packageMetadata.Version.ToNormalizedStringChecked(), installLocation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gep13 we had discussed making these optional, I found when I did that, I could no longer build CLE with these artifacts as it couldn't determine which method signature to use. Making them both mandatory solved that compile time issue for me. More than happy to be shown a better way to obsolete things here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep things working with an optional parameter, I think you would need to do the following:
public PackageResult(IPackageMetadata packageMetadata, IPackageSearchMetadata packageSearch, string installLocation, string sourceUri, string source = null)
But since we are creating a new constructor, I don't see any harm in making them non optional.
@AdmiringWorm might have some thoughts on this as well, so happy to let him chime in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be could to add some documentation for this method as well, especially around why there are two parameters related to Source, i.e. one to capture all the sources used during the operation, and one for the source that was actually used to perform the operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an overload would be better than using an optional parameter.
Adding more optional parameters would require a new compilation of CLE for it to work against it.
An optional parameter is only optional during compilation, but during runtime it is still a required to be passed, so when CLE is compiled against this constructor with the optional parameter, it will pass along null
as the value for the source
parameter, but as this wouldn't be present without a recompile, then the method signature will be different than what CLE expects.
At least, those are my understandings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be could to add some documentation for this method as well, especially around why there are two parameters related to Source, i.e. one to capture all the sources used during the operation, and one for the source that was actually used to perform the operation.
I'm not 100% sure on this one, but I have added some details to this particular overload.
public bool IsPinned { get; set; } | ||
public string ExtraInformation { get; set; } | ||
public string DeploymentLocation { get; set; } | ||
public string PackageInstalledFrom { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pauby can you provide any input here? This is with regard to capturing which specific source a package was downloaded and installed from. Thanks!
{ | ||
} | ||
|
||
public PackageResult(IPackageMetadata packageMetadata, IPackageSearchMetadata packageSearch, string installLocation, string source, string sourceUri) : this(packageMetadata.Id, packageMetadata.Version.ToNormalizedStringChecked(), installLocation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep things working with an optional parameter, I think you would need to do the following:
public PackageResult(IPackageMetadata packageMetadata, IPackageSearchMetadata packageSearch, string installLocation, string sourceUri, string source = null)
But since we are creating a new constructor, I don't see any harm in making them non optional.
@AdmiringWorm might have some thoughts on this as well, so happy to let him chime in here.
} | ||
|
||
Source = sources.FirstOrDefault(uri => uri.IsFile || uri.IsUnc).ToStringSafe(); | ||
SourceUri = sourceUri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the naming that is decided for the above parameter, we might want to make this one more explicit as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting that we add a new public property called SourceInstalledFrom
and use it instead of the existing SourceUri
?
(for now I will proceed with that being the idea, we can adjust it easily enough if not...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that wasn't what I was suggesting no, as I hadn't noticed (or rather, I had forgotten that SourceUri
was already a property on this class. I think what you have done here is fine, I would go one step further though and say that if SourceUri
is not being used anywhere, we mark it as Obsolete
, so that going forward, there is no ambiguity about what is going on with this class. Source
will be used (when needed) to store all the sources that were available when the operation took place, and SourceInstalledFrom
used to store where the package was actually installed from.
Thoughts?
{ | ||
} | ||
|
||
public PackageResult(IPackageMetadata packageMetadata, IPackageSearchMetadata packageSearch, string installLocation, string source, string sourceUri) : this(packageMetadata.Id, packageMetadata.Version.ToNormalizedStringChecked(), installLocation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be could to add some documentation for this method as well, especially around why there are two parameters related to Source, i.e. one to capture all the sources used during the operation, and one for the source that was actually used to perform the operation.
This commit adds the source location for a package to the .chocolatey directory, and adds the printing of the location to the `choco list` verbose output (and also `choco info --local-only`).
This commit reverts c74a19c where we had reverted the initial work to display the deployment location. As we are releasing a new version of CLE that can take this newer version as a breaking change, we can add this functionality in there too.
From a functionality standpoint I'm happy to approve this. I'll leave the final approval on naming and such to Gary and Paul before we call it all good, though. :3 |
I noticed some missed renames after @vexx32 left their review. I've added one last commit, and run the Chocolatey Agent Test Kitchen suite here: https://sra-soteria-web.ch0.co/buildConfiguration/TestKitchen_ChocolateyAgent/38694. I will review the results in the morning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me, just a couple minor nitpicks, and an overall suggestion on how to finish this off.
ChocolateyPackageMetadata packageLocalMetadata; | ||
string packageInstallLocation = null; | ||
string deploymentlocation = null; | ||
string packageInstalledFrom = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get this updated to the name that we went for?
package.Description.EscapeCurlyBraces().Replace("\n ", "\n").Replace("\n", "\n "), | ||
!string.IsNullOrWhiteSpace(package.ReleaseNotes.ToStringSafe()) ? "{0} Release Notes: {1}".FormatWith(Environment.NewLine, package.ReleaseNotes.EscapeCurlyBraces().Replace("\n ", "\n").Replace("\n", "\n ")) : string.Empty, | ||
!string.IsNullOrWhiteSpace(deploymentlocation) ? "{0} Deployed to: '{1}'".FormatWith(Environment.NewLine, deploymentlocation) :string.Empty, | ||
!string.IsNullOrWhiteSpace(packageInstalledFrom) ? "{0} Package Installed From: '{1}'".FormatWith(Environment.NewLine, packageInstalledFrom) : string.Empty, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!string.IsNullOrWhiteSpace(packageInstalledFrom) ? "{0} Package Installed From: '{1}'".FormatWith(Environment.NewLine, packageInstalledFrom) : string.Empty, | |
!string.IsNullOrWhiteSpace(packageInstalledFrom) ? "{0} Package installed from: '{1}'".FormatWith(Environment.NewLine, packageInstalledFrom) : string.Empty, |
I don't "know" is this is right or not. Perhaps a final check from @pauby here?
$Output.ExitCode | Should -Be 0 -Because $Output.String | ||
} | ||
|
||
It "Should contain a summary" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a copy/pasta or intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a copy/pasta... But it could probably be clearer about what summary it's looking for...
/// <param name="packageMetadata"></param> | ||
/// <param name="packageSearch"></param> | ||
/// <param name="installLocation"></param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should fill these out while we are here as well.
This adds pester tests to ensure the deployment location and the source repository are correctly output when running `choco info --local-only`
This commit renames the SourceInstalledFrom variable to the decided variable name. This also removes the uses of the deprecated PackageResult constructor.
This commit adds documentation to the PackageResult class, and slightly adjusts the formatting of the constructors to make them clearer.
This is a draft PR while we work through some of the PR changes and to ensure we don't forget to rebase the commits before merging.
Description Of Changes
Add the source repository and deployment location to the stored Chocolatey package information, displaying it as part of
choco info --local-only
andchoco list --verbose
.Motivation and Context
See #3804 and #1901
Testing
choco install <packageName>
$env:ChocolateyInstall/.chocolatey/<packageName>.<packageVersion>/.sourceInstalledFrom
that it exists, and includes the repository that was used for installation.choco install <packageName> --source .
$env:ChocolateyInstall/.chocolatey/<packageName>.<packageVersion>/.sourceInstalledFrom
that it exists, and includes the local path that was used for installation.choco info <packageName> --local-only
and ensure that both the deployment location and the source installed from location are listed.choco list --verbose
and ensure that both the deployment location and the source installed from location are listed.Operating Systems Testing
Change Types Made
Change Checklist
Related Issue
choco info
output #1901